-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix arbitrary file access during archive extraction ("Zip Slip") #34982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Assigning reviewers: R: @jrmccluskey for label go. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Reminder, please take a look at this pr: @jrmccluskey |
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @lostluck for label go. Available commands:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lovely addition. Could you please add a unit test for the behavior?
Reminder, please take a look at this pr: @lostluck |
waiting on author |
hi @lostluck seems like i see |
Well yes. They were passing before. Because we didn't have any tests that validate this zip slip problem. That just means that your change didn't break existing tests. What I'd like to see is for there to be a test that catches this zip slip behavior so we don't break the safety you're adding in the future. |
beam/sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go
Lines 142 to 164 in 75cf7e1
To fix the issue, we need to validate the file paths extracted from the zip archive to ensure they do not contain directory traversal elements (
..
) and are confined to the intended destination directory. This can be achieved by resolving the absolute path of the constructedfileName
and ensuring it is a subpath of thedest
directory. If the validation fails, the file should be skipped or an error should be raised.The fix involves:
fileName
usingfilepath.Abs
.dest
directory.Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated. archive paths.
Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (
..
). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.For example, if a zip file contains a file entry
..\beam-file
, and the zip file is extracted to the directoryc:\output
, then naively combining the paths would result in an output file path ofc:\output\..\beam-file
, which would cause the file to be written toc:\beam-file
.In this an archive is extracted without validating file paths. If archive.zip contained relative paths (for instance, if it were created by something like zip archive.zip ../file.txt) then executing this code could write to locations outside the destination directory.
To fix this vulnerability, we need to check that the path does not contain any "
..
" elements in it.References
Zip Slip Vulnerability
Path Traversal
CWE-22
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.